Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: add integration gh action #509

Merged
merged 1 commit into from
Jan 30, 2024

Conversation

aaraney
Copy link
Member

@aaraney aaraney commented Jan 29, 2024

Add github action that runs integration tests. At the moment, integration tests are run only on python 3.10. Its up for discussion what version or versions of python we run these tests on and the frequency at which we run the tests.

@aaraney
Copy link
Member Author

aaraney commented Jan 29, 2024

These should fail because our integration tests have some regressions.

@christophertubbs
Copy link
Contributor

Why is this only added to 3.10? Sure, it fails, but one for all and all for one, amiright? May as well blow up all of them if we can't get it functioning for one of them.

@aaraney
Copy link
Member Author

aaraney commented Jan 30, 2024

@christophertubbs, yeah I think that is a fair point. They don't take that long to run since we don't have too too many of them and we do a fair amount of caching. My thinking was it most important to test on the version of python that we have in production. Im not sure if using a build matrix in this context will work, but let me force push something up and see what happens.

Copy link
Contributor

@christophertubbs christophertubbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything seems to fail correctly, so I'm pretty happy with the changes

@christophertubbs christophertubbs merged commit 4c31ad8 into NOAA-OWP:master Jan 30, 2024
4 of 8 checks passed
@aaraney
Copy link
Member Author

aaraney commented Jan 30, 2024

Thanks for reviewing this, @christophertubbs!

@robertbartel
Copy link
Contributor

robertbartel commented Feb 1, 2024

@aaraney, I'm not sure yet, but this may need to be reverted.

The integration tests weren't intended to be run in Github. Since they were meant to test how things worked together, at least some of the tests depend on certain parts of a DMOD deployment being initialized that we wouldn't want to try to do in Action runners, like create some of the Docker networks or a running object store stack.

We might be able to pull things apart, but I think those end up being sophisticated mocks for unit tests rather than integration test.

It's also possible I'm overloading and/or misusing the term here, but regardless, as they are now, the "integration" tests run by the current tools were only expected to be run locally.

@aaraney
Copy link
Member Author

aaraney commented Feb 1, 2024

It's also possible I'm overloading and/or misusing the term here, but regardless, as they are now, the "integration" tests run by the current tools were only expected to be run locally.

I think that is a fair interpretation. For better or worse IMO the interpretation of "integration" tests probably varies from one person to the next more than unit or end2end testing. To me, integration testing tests a subsystem or subsystems. That could include things like networking, but I don't think its inherently required to be considered an integration test.

My thinking was the benefits still greatly out weight the costs of running some form of our integration tests using Github actions. Right, we might not catch issues stemming from the interaction with our deployment environment and our integration tests, but nothing is stopping us from running them in both places. Just here, we are guaranteed that they are being run frequently. I guess I see this as a why not? If we can use github actions and service containers to get us 85% of the way there, why not?

@christophertubbs
Copy link
Contributor

We need integration tests that can 'prove' component A,B, and C can talk to an object store (minio) and key-value store/message queue (redis). Testing docker stuff is always good (especially since this... you know... is meant to manipulate containers), but docker doesn't matter if bare components can't integrate with the elements they need to integrate with. Docker is a means to an end - using a swarm gets us exactly what we need in a deployment environment. We won't be able to test if the scheduler or whatever works here since it'd have to spin up new containers, but we need to ensure that everything else works.

Call them smoke tests, call them Tubbs-is-dumb tests; it doesn't matter. We need tests that verify that our products not only pass their own unit tests but can also work with crucial 3rd party tools. These tests do that.

I'm personally against reversion here. @robertbartel , if you feel that borders are being crossed then the scripts themselves need to be altered to take docker/containerization out of the equation, not removing the actions themselves. If the integration tests test component-3rd party cooperation and docker, we need to separate docker tests into their own set.

We need to stay away from tests that are only meant to run locally - all those would end up being are "works on my machine" tests. I separate that idea from 'docker environment' tests, especially since this product will eventually need K8 environment tests (coming 2087!).

Just about the only reason I can think of to remove these tests is if they somehow cause us to exceed compute limits imposed by github itself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants